Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: code coverage incomplete on redirect #660

Merged

Conversation

fvclaus
Copy link
Contributor

@fvclaus fvclaus commented May 26, 2023

We have a frontend that, upon loading, redirects the user to Keycloak. The user logs in and is returned to the application. The code coverage written to the disk was incomplete and after hours of debugging I found the issue. The problem is the caching of references to the window.__coverage__ object. When the code that redirects the user is instrumented, the covered statements will be different for the initial redirect and the subsequent return so this plugin will correctly recognize them as different and will keep a reference to both coverage objects. When the code that redirects the user is not instrumented (because it is a library), the coverage object will be identical. Both coverage objects will have identical statement coverage and the current version of the code will throw away the second coverage object, keeping only the reference to the first one which is not updated anymore. This will result in a code coverage where only the statments until the call to the library are covered.

Before:

----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
----------|---------|----------|---------|---------|-------------------
All files |      50 |      100 |       0 |      50 |                   
 utils.js |      50 |      100 |       0 |      50 | 2                 
----------|---------|----------|---------|---------|-------------------
ERROR: Coverage for lines (50%) does not meet global threshold (100%)

After:

----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
----------|---------|----------|---------|---------|-------------------
All files |     100 |      100 |     100 |     100 |                   
 utils.js |     100 |      100 |     100 |     100 |                   
----------|---------|----------|---------|---------|-------------------

Keywords for Google: Code coverage different in afterEach.

@CLAassistant
Copy link

CLAassistant commented May 26, 2023

CLA assistant check
All committers have signed the CLA.

.nycrc.json Outdated
@@ -1,6 +1,7 @@
{
"exclude": [
"support-utils.js",
"task-utils.js"
"task-utils.js",
"support.js"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this being excluded?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you run the unit-tests, support.js is reported as having too little coverage, but support.js is not even being tested (only support-utils.js). I thought that this is an issue with my code, but I get the same result on the main branch.

@mschile mschile assigned cacieprins and unassigned mschile Jun 23, 2023
@fvclaus
Copy link
Contributor Author

fvclaus commented Sep 18, 2023

@mschile , @cacieprins This MR has been hanging around for almost 4 months. Is there something I can do to get this merged?

@mschile
Copy link
Contributor

mschile commented Oct 13, 2023

Hi @fvclaus 👋, sorry for the delay. I will try to get back to this very soon.

@mschile mschile assigned mschile and unassigned cacieprins Oct 13, 2023
@mschile mschile changed the title Coverage incomplete on redirect / reload fix: code coverage incomplete on redirect Oct 23, 2023
Copy link
Member

@emilyrohrbough emilyrohrbough left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fvclaus Nice job tracking down the root-cause of this issues. We appreciate the contribution!

@emilyrohrbough emilyrohrbough merged commit 443029b into cypress-io:master Oct 25, 2023
@cypress-app-bot
Copy link

🎉 This PR is included in version 3.12.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants